MCP: wait-for-ready + transient retry on first call; add tests #6894
Conversation
…sient -32000 'Connection closed'; add tests
dosubot
bot
added
size:L
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| timeoutMs: number = 10000, | ||
| ): Promise<ConnectedMcpConnection> { | ||
| // Immediately error if there's no connected-type connection to wait on (preserves legacy error for tests/UX) | ||
| const initial = this.findConnection(serverName, source) |
There was a problem hiding this comment.
Is this intentional? The connection check happens outside the loop, which could lead to a race condition if the connection is removed between the initial check and entering the loop. Consider moving the findConnection call inside the loop for safer operation.
| } catch (error) { | ||
| // Handle initial transient "Connection closed" on first invocation after install/start | ||
| if (this.isTransientClosedError(error)) { | ||
| await delay(200) |
There was a problem hiding this comment.
Could we make this retry delay configurable or at least define it as a constant? The magic number 200ms appears in both callTool and readResource. Something like: private static readonly TRANSIENT_ERROR_RETRY_DELAY_MS = 200;
| const msg = error instanceof Error ? error.message : String(error) | ||
| if (typeof code === "number" && code === -32000) return true | ||
| if (typeof code === "string" && /-?32000/.test(code)) return true | ||
| return /connection closed|closed before|socket hang up|ECONNRESET|EPIPE|ERR_STREAM_PREMATURE_CLOSE/i.test( |
There was a problem hiding this comment.
This regex pattern is quite broad and might catch unrelated errors. Could we be more specific about which error patterns should trigger a retry? For example, we might want to exclude certain EPIPE errors that indicate permanent failures rather than transient issues.
| mkdir: vi.fn().mockResolvedValue(undefined), | ||
| })) | ||
|
|
||
| describe("McpHub transient-retry and readiness", () => { |
There was a problem hiding this comment.
Great test coverage for the happy path! Consider adding edge case tests: What happens when waitForServerReady times out? What happens on the second retry failure (should not retry a third time)? Error handling when connection is removed during the wait period. These would help ensure the retry logic is robust under various failure scenarios.
hannesrudolph
added
the
Issue/PR - Triage
|
Closing for now since this doesn't have an issue that I can repro and test against |
Changes:\n- Add waitForServerReady to guard against race conditions on initial connect.\n- Add transient error detection (-32000/connection closed) and single retry for tools/call and resources/read.\n- Update callTool and readResource to use readiness + retry.\n- Add new tests (McpHub.retry.spec.ts) covering readiness wait and transient retry.\n- All MCP tests pass (McpHub.spec.ts, useMcpToolTool.spec.ts, and new retry spec).\n\nThis improves robustness when an MCP server has just started or after install, reducing flaky 'Connection closed' errors on the first request.
Important
Enhances
McpHubwith server readiness checks and transient error retries, adding tests for robustness.waitForServerReadyinMcpHub.tsto ensure server readiness before initial requests.-32000/connection closederrors incallToolandreadResource.callToolandreadResourceto use readiness check and retry logic.isTransientClosedErrorto identify transient errors.McpHub.retry.spec.tsfor readiness wait and transient retry.callToolandreadResourceretry once on transient errors.McpHub.spec.tsanduseMcpToolTool.spec.ts.This description was created by
for 69f66b6. You can customize this summary. It will automatically update as commits are pushed.